Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use crossterm cursor when terminal loses focus #6858

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Use crossterm cursor when terminal loses focus #6858

merged 1 commit into from
Nov 27, 2023

Conversation

hnorkowski
Copy link
Contributor

@hnorkowski hnorkowski commented Apr 23, 2023 •
edited
Loading

All but the documents block cursor change their shape to a box outline if the terminal loses focus. I am missing that feature because (depending on your window manager) it can be hard to tell which editor has focus if you have multiple open at the same time. Because the custom rendered block cursor is just the background color of the respective char it is no possible to achieve the same behavior (display as outline).

Therefore I created a proof of concept for the custom block cursor to change its color when the terminal window loses focus.

If the theme does not define a color for the unfocused cursor (e.g. ui.cursor.primary.unfocused) it should calculate the color by faking transparency of the cursor. Therefore it tries to get the background color and the cursor color and mix them.

Currently I have 2 main problems with the color calculation:

  • I do not know how to get the RGB values of a named color (e.g. yellow) but I need those to calculate the "transparent" cursor color
  • Some themes (e.g. default) do not define a cursor color (Style.bg is None). There are default colors defined but I do not know how I can query them.
  • I also tried to use the DIM modifier but it did not seem to have any effect (atleast in alacritty). I forgot to create a commit for that but if desired I could reimpl. that to analyze why it did not work.

The current implementation works well with some themes, e.g. gruvbox, catpuccin_*, emacs.

Another questions that should be discussed:

  • If the feature gets accepted, should it be the default?
    • I think yes because all other cursors have this behavior

@pascalkuthe pascalkuthe added the S-waiting-on-review Status: Awaiting review from a maintainer. label Apr 25, 2023
the-mikedavis
the-mikedavis reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Named colors (cyan, yellow, red, etc.) are defined by the terminal (for example https://sw.kovidgoyal.net/kitty/conf/#the-color-table) and there's no way to query their value from a terminal application. I don't think we should be blending colors anyways though: all colors should be written out explicitly in a theme

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
helix-view/src/theme.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 13, 2023
@archseer
Copy link
Member

Hmm this is probably a better idea than what I had: I simply hid the cursor. But that makes it a bit confusing when the window is out of focus since no other editor would behave that way

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 13, 2023
pascalkuthe
pascalkuthe reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It would be nice if you could define an unfocused color for the default theme tough (the rest of the themes can be changed by their author but it's nice if the default works)

the-mikedavis
the-mikedavis reviewed Jun 13, 2023
View reviewed changes
book/src/themes.md Outdated Show resolved Hide resolved
book/src/themes.md Outdated Show resolved Hide resolved
helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
@hnorkowski
Copy link
Contributor Author

This looks good to me. It would be nice if you could define an unfocused color for the default theme tough (the rest of the themes can be changed by their author but it's nice if the default works)

Yeah I am trying to but I cannot figure out the default theme keys. The cursor has different colors depending on if its on a char or a whitespace and cannot figure out where this behavior is defined the theme.toml. With my current impl. the cursor will always have the same color when out of focus.

@pascalkuthe
Copy link
Member

This looks good to me. It would be nice if you could define an unfocused color for the default theme tough (the rest of the themes can be changed by their author but it's nice if the default works)

Yeah I am trying to but I cannot figure out the default theme keys. The cursor has different colors depending on if its on a char or a whitespace and cannot figure out where this behavior is defined the theme.toml. With my current impl. the cursor will always have the same color when out of focus.

That's default theme uses "ui.cursor" = { modifiers = ["reversed"] } which just swaps foreground and background color. Even if you keep reversed changing the bg color would involve overwiting the fg color so defining different colors for whithespaces is not possible. I think that's ok this distinction isn't that important if the editor isn't focused.

@hnorkowski
Copy link
Contributor Author

But this creates the problem that I need to chose a color that works well for all colors the cursor can have. This is kinda tricky. I will try to find a good one.

@pascalkuthe pascalkuthe marked this pull request as ready for review June 13, 2023 22:04
@pascalkuthe
Copy link
Member

I marked this as ready for review as we are already reviewing. If you feel this is not ready yet feel free to change the status again. There seem to be some problems with the default theme and lints that need fixing (see CI)

@hnorkowski hnorkowski force-pushed the unfocused_block_cursor branch 2 times, most recently from 0ca9c95 to d41446f Compare June 13, 2023 23:25
@hnorkowski
Copy link
Contributor Author

I think this is ready. I updated the catppuccin mocha theme to support this feature aswell.

The only thing that could need work is the default theme. You guys might wanna check it out and maybe even try if you like other colors more.

the-mikedavis
the-mikedavis reviewed Jun 16, 2023
View reviewed changes
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, just a minor comment. We'll also want to get @archseer's review since this touches the default theme

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis changed the title Feat(ui): Block cursor change color when terminal loses focuse Change cursor color when terminal loses focus Jun 16, 2023
the-mikedavis
the-mikedavis previously approved these changes Jun 17, 2023
View reviewed changes
pascalkuthe
pascalkuthe previously approved these changes Jun 17, 2023
View reviewed changes
@hnorkowski hnorkowski force-pushed the unfocused_block_cursor branch 2 times, most recently from bb46d07 to a9d5df1 Compare November 23, 2023 15:58
the-mikedavis
the-mikedavis previously approved these changes Nov 25, 2023
View reviewed changes
@hnorkowski
Copy link
Contributor Author

hnorkowski commented Nov 25, 2023 •
edited
Loading

This is not ready yet. I think we should go back to draft. I want to try this idea:

So my idea is: Why not just change the cursor type to bar while helix has no focus and back to block when its in focus again?

However I am a bit lost in the code base. I was yet not able to find the correct place to overwrite the cursor. I found how to overwrite the cursor when the crossterm cursor is in use but I could not figure out how to switch between crossterm cursor and custom cursor. Unfortunately I didn't had much time recently. Maybe you have a hint for me?

@the-mikedavis
Copy link
Member

helix/helix-term/src/application.rs

Lines 1152 to 1156 in 8c68074

use helix_view::graphics::CursorKind;
self.terminal
.backend_mut()
.show_cursor(CursorKind::Block)
.ok();
is a good example of explicitly setting a block cursor

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 25, 2023
@hnorkowski
Copy link
Contributor Author

hnorkowski commented Nov 25, 2023 •
edited
Loading

I finally managed to impl. the behavior. It seems to work flawless.

The only point left to discuss: Should we also do something about secondary cursors? Currently only the main cursor is changing when losing focus. I think for me thats ok. What is your opinion?

@the-mikedavis
Copy link
Member

I think we can leave secondary cursors as-is. We can't make them bars (we just fill in a background to make them look like blocks normally) like the primary cursor. We could add theming for them when unfocused like the original change in this PR if the bar cursor isn't clear enough. That could probably be a follow-up though

@hnorkowski
Copy link
Contributor Author

hnorkowski commented Nov 26, 2023 •
edited
Loading

Yeah I think you are right that it could be a follow-up after using this for some time and see if it would be helpful.

With that I think the PR is ready. Should I update the initial PR message?

@hnorkowski hnorkowski changed the title Change cursor color when terminal loses focus Editor: always use crossterm cursor when unfocused for constistent behaviour Nov 26, 2023
the-mikedavis
the-mikedavis reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit about a comment.

I like this behavior - it looks nice to me on Kitty for single panes and splits

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis changed the title Editor: always use crossterm cursor when unfocused for constistent behaviour Use crossterm cursor when terminal loses focus Nov 27, 2023
@hnorkowski
feat(ui): use crossterm cursor in editor when out of focus
3aff043 
Use crossterm cursor in the editor when the terminal is out of focus to achieve consistent out-of-focus cursor behaviour
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2023
@pascalkuthe pascalkuthe merged commit 71fd858 into helix-editor:master Nov 27, 2023
6 checks passed
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
@hnorkowski @dgkf
use crossterm cursor in editor when out of focus (helix-editor#6858)
077a0c5 
Use crossterm cursor in the editor when the terminal is out of focus to achieve consistent out-of-focus cursor behaviour
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
@hnorkowski @mtoohey31
use crossterm cursor in editor when out of focus (helix-editor#6858)
1214778 
Use crossterm cursor in the editor when the terminal is out of focus to achieve consistent out-of-focus cursor behaviour
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
@hnorkowski @Vulpesx
use crossterm cursor in editor when out of focus (helix-editor#6858)
763d0bb 
Use crossterm cursor in the editor when the terminal is out of focus to achieve consistent out-of-focus cursor behaviour
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@hnorkowski
use crossterm cursor in editor when out of focus (helix-editor#6858)
add056f 
Use crossterm cursor in the editor when the terminal is out of focus to achieve consistent out-of-focus cursor behaviour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe approved these changes

@the-mikedavis the-mikedavis the-mikedavis approved these changes

@archseer archseer Awaiting requested review from archseer

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
next
Development

Successfully merging this pull request may close these issues.
4 participants
@hnorkowski @archseer @pascalkuthe @the-mikedavis